refactor + new account sync metrics + isolating health status inside folder admin-panel > health-status #10314
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces significant changes to the health status monitoring system, focusing on account synchronization and component organization.
- Replaces message sync with comprehensive account sync monitoring in
/packages/twenty-server/src/engine/core-modules/health/indicators/account-sync.health.ts, adding calendar sync metrics - Reorganizes health status components into dedicated
/health-statusfolder structure for better code organization - Simplifies
SystemHealthDTO in/packages/twenty-server/src/engine/core-modules/admin-panel/dtos/system-health.dto.tsto use direct status enums - Adds calendar sync health monitoring with 20% failure rate threshold and timeout handling
- Integrates health monitoring into calendar event import system via
HealthModulein related modules
Note: The PR description references issues about keyboard shortcuts and soft focus mode, but the actual changes are unrelated to those issues.
39 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile
| the service is down | ||
| </StyledErrorMessage> | ||
| ) : ( | ||
| <StyledDetailsContainer>{formattedDetails}</StyledDetailsContainer> |
There was a problem hiding this comment.
style: formattedDetails could be null here. Consider adding a fallback message when no details are available.
...t/src/modules/settings/admin-panel/health-status/components/DatabaseAndRedisHealthStatus.tsx
Outdated
Show resolved
Hide resolved
...ettings/admin-panel/health-status/components/SettingsAdminHealthAccountSyncCountersTable.tsx
Outdated
Show resolved
Hide resolved
| details, | ||
| title, | ||
| }: { | ||
| details: Record<string, any> | null; |
There was a problem hiding this comment.
style: Using Record<string, any> is too permissive. Consider creating a specific type for the details object structure
...wenty-front/src/modules/settings/admin-panel/health-status/components/AccountSyncMetrics.tsx
Outdated
Show resolved
Hide resolved
| accountSyncHealth.isHealthy.mockRejectedValue( | ||
| new Error(HEALTH_ERROR_MESSAGES.MESSAGE_SYNC_CHECK_FAILED), | ||
| ); |
There was a problem hiding this comment.
logic: Using MESSAGE_SYNC_CHECK_FAILED error for accountSync is inconsistent with the service name change. Should use a new ACCOUNT_SYNC_CHECK_FAILED error message.
| const currentCounter = | ||
| await this.cacheStorage.get<AccountSyncJobByStatusCounter>(cacheKey); | ||
|
|
||
| const updatedCounter = { | ||
| ...(currentCounter || {}), | ||
| [status]: (currentCounter?.[status] || 0) + increment, | ||
| }; |
There was a problem hiding this comment.
style: Duplicate code pattern between message and calendar sync counters. Consider extracting to a shared method to reduce duplication.
...wenty-server/src/engine/core-modules/health/indicators/__tests__/account-sync.health.spec.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/health/indicators/account-sync.health.ts
Outdated
Show resolved
Hide resolved
...r/src/modules/calendar/calendar-event-import-manager/calendar-event-import-manager.module.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/health/indicators/account-sync.health.ts
Outdated
Show resolved
Hide resolved
...server/src/engine/core-modules/admin-panel/dtos/admin-panel-indicator-health-status.input.ts
Outdated
Show resolved
Hide resolved
...t/src/modules/settings/admin-panel/health-status/components/SettingsHealthStatusListCard.tsx
Outdated
Show resolved
Hide resolved
| const services = [ | ||
| { | ||
| id: 'DATABASE', | ||
| name: 'Database Status', |
There was a problem hiding this comment.
Also use this in breadcrumb?
There was a problem hiding this comment.
I removed this from the front, and added it as a const in the back itself to have single source of truth, may be you want to check the descriptions, I just have them as a generic sentence as of now
| : AdminPanelHealthServiceStatus.OUTAGE; | ||
| } | ||
|
|
||
| private transformServiceDetails(details: any) { |
There was a problem hiding this comment.
@FelixMalfait, I am keeping this asany for now, as there is no proper structure to it yet!
To be improved upon in upcoming PRs
There was a problem hiding this comment.
Ok to do it in a followup PR 👍
| <> | ||
| {errorMessages.length > 0 && ( | ||
| <StyledErrorMessage> | ||
| {`${errorMessages.join(' and ')} ${errorMessages.length > 1 ? 'are' : 'is'} not available because the service is down`} |
There was a problem hiding this comment.
We should use t`` for translatable strings
| {!isMessageSyncDown && parsedDetails.messageSync?.details && ( | ||
| <SettingsAdminHealthAccountSyncCountersTable | ||
| details={parsedDetails.messageSync.details} | ||
| title="Message Sync Status" |
There was a problem hiding this comment.
Other example of translatable string
|
Thanks @ehconitin for your contribution! |
closes twentyhq/core-team-issues#444
twentyhq/core-team-issues#443
twentyhq/core-team-issues#442